Log search functionality and log browser refactoring by ezaquarii · Pull Request #4309 · nextcloud/android

ezaquarii · 2019-08-11T12:18:22Z

added log search in logs browser
added logs browser view model test
added universal async filtering utility
refactored async runner and added manual runner
migrated logs browser to Android DataBinding and Kotlin
disabled imports ordering Ktlint rule as IDE does not
support ktlint ordering
added some missing tests around logger

Signed-off-by: Chris Narkiewicz hello@ezaquarii.com

ezaquarii · 2019-08-11T12:19:29Z

@tobiasKaminsky @AndyScherzinger This is a follow-up on #4275 with discussed UI improvements.

AndyScherzinger · 2019-08-11T12:27:50Z

@ezaquarii I haven't had a chance to test it yet, first review looks good code-wise. Did you take care of themeing (search icon / search view)? If not @tobiasKaminsky might be able to help :)

codecov · 2019-08-11T15:36:21Z

Codecov Report

Merging #4309 into master will increase coverage by 0.19%.
The diff coverage is 62.74%.

@@             Coverage Diff              @@
##             master    #4309      +/-   ##
============================================
+ Coverage     16.63%   16.83%   +0.19%     
  Complexity        1        1              
============================================
  Files           351      354       +3     
  Lines         31408    31498      +90     
  Branches       4446     4471      +25     
============================================
+ Hits           5225     5302      +77     
  Misses        25308    25308              
- Partials        875      888      +13

Impacted Files	Coverage Δ	Complexity Δ
...java/com/nextcloud/client/di/ComponentsModule.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...owncloud/android/ui/activity/SettingsActivity.java	`38.44% <ø> (ø)`	`0 <0> (ø)`	⬇️
...c/main/java/com/nextcloud/client/di/AppModule.java	`72.72% <0%> (ø)`	`0 <0> (ø)`	⬇️
...in/java/com/owncloud/android/utils/ThemeUtils.java	`51.89% <0%> (-0.67%)`	`0 <0> (ø)`
...ava/com/nextcloud/client/logger/ui/LogsActivity.kt	`0% <0%> (ø)`	`0 <0> (?)`
...java/com/nextcloud/client/logger/ui/LogsAdapter.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...java/com/nextcloud/client/logger/FileLogHandler.kt	`92.45% <100%> (+0.45%)`	`0 <0> (ø)`	⬇️
src/main/java/com/nextcloud/client/core/Task.kt	`100% <100%> (ø)`	`0 <0> (?)`
...com/nextcloud/client/core/ThreadPoolAsyncRunner.kt	`100% <100%> (ø)`	`0 <0> (?)`
...va/com/nextcloud/client/logger/ui/LogsViewModel.kt	`60.78% <67.44%> (+60.78%)`	`0 <0> (ø)`	⬇️
... and 10 more

codecov · 2019-08-11T15:36:21Z

Codecov Report

Merging #4309 into master will increase coverage by 0.19%.
The diff coverage is 62.33%.

@@             Coverage Diff             @@
##             master   #4309      +/-   ##
===========================================
+ Coverage      16.6%   16.8%   +0.19%     
  Complexity        1       1              
===========================================
  Files           351     354       +3     
  Lines         31410   31499      +89     
  Branches       4447    4472      +25     
===========================================
+ Hits           5215    5292      +77     
+ Misses        25317   25316       -1     
- Partials        878     891      +13

Impacted Files	Coverage Δ	Complexity Δ
...java/com/nextcloud/client/di/ComponentsModule.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...owncloud/android/ui/activity/SettingsActivity.java	`38.44% <ø> (ø)`	`0 <0> (ø)`	⬇️
...c/main/java/com/nextcloud/client/di/AppModule.java	`72.72% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ava/com/nextcloud/client/logger/ui/LogsActivity.kt	`0% <0%> (ø)`	`0 <0> (?)`
...in/java/com/owncloud/android/utils/ThemeUtils.java	`51.89% <0%> (-0.67%)`	`0 <0> (ø)`
...java/com/nextcloud/client/logger/ui/LogsAdapter.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
src/main/java/com/nextcloud/client/core/Task.kt	`100% <100%> (ø)`	`0 <0> (?)`
...com/nextcloud/client/core/ThreadPoolAsyncRunner.kt	`100% <100%> (ø)`	`0 <0> (?)`
...java/com/nextcloud/client/logger/FileLogHandler.kt	`92.45% <100%> (+0.45%)`	`0 <0> (ø)`	⬇️
...va/com/nextcloud/client/logger/ui/LogsViewModel.kt	`60.78% <67.44%> (+60.78%)`	`0 <0> (ø)`	⬇️
... and 17 more

nextcloud-android-bot · 2019-08-11T16:40:01Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10354

ezaquarii · 2019-08-11T17:09:34Z

@AndyScherzinger @tobiasKaminsky I tried to copy the solution from ExtendedListFragment.onCreateOptionsMenu but the search view won't budge - it's still dimmed. I'm not sure how the theming is done there, but it looks like it depends on some hidden state set elsewhere.

nextcloud-android-bot · 2019-08-12T14:45:00Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10365

AndyScherzinger · 2019-08-12T16:52:53Z

@tobiasKaminsky can you provide @ezaquarii with the infos for search bar theming (which you already implemented elsewhere in the app)? :)

tobiasKaminsky · 2019-08-13T06:21:07Z

Regarding translation strings:
Please do not rename keys that are already in use, otherwise they will conflict with older version (we translate master and 1-2 older versions within the same transifex repository)
Just create a new string, even when it is with the same content. It causes overhead to the translation team, but this way we are bulletproof.

As transifex is using only values/strings.xml as source, we do not need to change the translated files. In best case they will be overwritten by next transifex update, and in worst case they result in a conflict.
Modifying only values/strings.xml makes review also a bit easier.

After that I am happy to review it (currently my browser refuses a bit to show that many changes…)

I will add both to our contributing.md

tobiasKaminsky · 2019-08-13T06:40:29Z

@AndyScherzinger @tobiasKaminsky I tried to copy the solution from ExtendedListFragment.onCreateOptionsMenu but the search view won't budge - it's still dimmed. I'm not sure how the theming is done there, but it looks like it depends on some hidden state set elsewhere.

@tobiasKaminsky can you provide @ezaquarii with the infos for search bar theming (which you already implement4ed elsewhere in the app)? :)

Sorry, I do not get the term "budge". What is wrong?
At a quick test, only three dots are tinted wrong on a light theme?
(not sure if this is even a problem with this PR, I doubt so…)

AndyScherzinger · 2019-08-13T11:21:37Z

@tobiasKaminsky - could you test if it works with (very) light Background and dark text/icons? Else it seems fine (except for the 3-dot icon)

tobiasKaminsky · 2019-08-14T07:08:38Z

I reverted translation files and rebased.
I'll review & test it today and then let's merge it.

After that I'll have a look again at the tinting problematic, as I think that the ThemeUtils.themeSearchView() is not working and thus it might be changed also in other places.

tobiasKaminsky

Only minor changes.

* added log search in logs browser * added logs browser view model test * added universal async filtering utility * refactored async runner and added manual runner * migrated logs browser to Android DataBinding and Kotlin * disabled imports ordering Ktlint rule as IDE does not support ktlint ordering * added some missing tests around logger Closes #4311 Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

…ip ci] Signed-off-by: nextcloud-android-bot <android@nextcloud.com>

nextcloud-android-bot · 2019-08-14T20:48:12Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10382.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

nextcloud-android-bot · 2019-08-14T20:53:08Z

Codacy

276

Lint

Type	master	PR
Warnings	59	58
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	24
Correctness Warnings	69
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	111
Security Warnings	46
Dodgy code Warnings	135
Total	410

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	24
Correctness Warnings	69
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	111
Security Warnings	46
Dodgy code Warnings	135
Total	410

AndyScherzinger · 2019-08-14T21:03:42Z

Ready, steady, merge 🌟

1d17b35 Merge pull request #4309 from nextcloud/ezaquarii/logs-search 725ffc4 Drone: update Lint results to reflect reduced error/warning count [skip ci] 92e2234 Merge commit 'c35873f5dcb77bf9a33111c1fee2d25584f6a22d' c35873f Log search functionality and log browser refactoring 496f101 Merge pull request #4324 from nextcloud/translate ceb3999 Extract strings to be translatable e48a874 Merge pull request #4322 from nextcloud/contributingLicense 05dcedb add licene sample c6abcd2 [tx-robot] updated from transifex d6eb9de daily dev 20190813

1d17b35 Merge pull request nextcloud#4309 from nextcloud/ezaquarii/logs-search 725ffc4 Drone: update Lint results to reflect reduced error/warning count [skip ci] 92e2234 Merge commit 'c35873f5dcb77bf9a33111c1fee2d25584f6a22d' c35873f Log search functionality and log browser refactoring 496f101 Merge pull request nextcloud#4324 from nextcloud/translate ceb3999 Extract strings to be translatable e48a874 Merge pull request nextcloud#4322 from nextcloud/contributingLicense 05dcedb add licene sample c6abcd2 [tx-robot] updated from transifex d6eb9de daily dev 20190813 Signed-off-by: alex <alex.plutta@googlemail.com>

ezaquarii requested review from AndyScherzinger and tobiasKaminsky August 11, 2019 12:18

ezaquarii force-pushed the ezaquarii/logs-search branch 2 times, most recently from 12ec2b5 to 59f770a Compare August 11, 2019 12:21

AndyScherzinger added the 3. to review label Aug 11, 2019

ezaquarii force-pushed the ezaquarii/logs-search branch from 59f770a to 0b77175 Compare August 11, 2019 12:25

ezaquarii force-pushed the ezaquarii/logs-search branch 3 times, most recently from 15677ea to 3c1c8be Compare August 11, 2019 15:36

nextcloud deleted a comment from nextcloud-android-bot Aug 11, 2019

ezaquarii force-pushed the ezaquarii/logs-search branch from 3c1c8be to daccd56 Compare August 11, 2019 15:48

nextcloud deleted a comment from nextcloud-android-bot Aug 11, 2019

ezaquarii force-pushed the ezaquarii/logs-search branch 2 times, most recently from 3c13da0 to 8dbb0d2 Compare August 11, 2019 16:19

nextcloud deleted a comment from nextcloud-android-bot Aug 11, 2019

ezaquarii force-pushed the ezaquarii/logs-search branch from 3e62bea to 2b143e8 Compare August 12, 2019 14:32

AndyScherzinger mentioned this pull request Aug 12, 2019

Log: do not quit view after reset #4311

Closed

tobiasKaminsky force-pushed the ezaquarii/logs-search branch from 35d086f to c6d14b8 Compare August 14, 2019 07:06

tobiasKaminsky approved these changes Aug 14, 2019

View reviewed changes

Comment thread src/main/java/com/nextcloud/client/core/Task.kt

Comment thread src/main/res/layout/logs_activity.xml

ezaquarii force-pushed the ezaquarii/logs-search branch from c6d14b8 to c35873f Compare August 14, 2019 19:21

drone and others added 2 commits August 14, 2019 20:38

Merge commit 'c35873f5dcb77bf9a33111c1fee2d25584f6a22d'

92e2234

Drone: update Lint results to reflect reduced error/warning count [sk…

725ffc4

…ip ci] Signed-off-by: nextcloud-android-bot <android@nextcloud.com>

AndyScherzinger approved these changes Aug 14, 2019

View reviewed changes

AndyScherzinger merged commit 1d17b35 into master Aug 14, 2019

delete-merged-branch Bot deleted the ezaquarii/logs-search branch August 14, 2019 21:03

AndyScherzinger added this to the Nextcloud App 3.8.0 milestone Aug 14, 2019

Uh oh!

Uh oh!

Conversation

ezaquarii commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezaquarii commented Aug 11, 2019

Uh oh!

AndyScherzinger commented Aug 11, 2019

Uh oh!

codecov Bot commented Aug 11, 2019

Codecov Report

Uh oh!

codecov Bot commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nextcloud-android-bot commented Aug 11, 2019

Uh oh!

ezaquarii commented Aug 11, 2019

Uh oh!

nextcloud-android-bot commented Aug 12, 2019

Uh oh!

AndyScherzinger commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasKaminsky commented Aug 13, 2019

Uh oh!

tobiasKaminsky commented Aug 13, 2019

Uh oh!

AndyScherzinger commented Aug 13, 2019

Uh oh!

tobiasKaminsky commented Aug 14, 2019

Uh oh!

tobiasKaminsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nextcloud-android-bot commented Aug 14, 2019

Uh oh!

nextcloud-android-bot commented Aug 14, 2019

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

Uh oh!

AndyScherzinger commented Aug 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ezaquarii commented Aug 11, 2019 •

edited

Loading

codecov Bot commented Aug 11, 2019 •

edited

Loading

AndyScherzinger commented Aug 12, 2019 •

edited

Loading